Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix render_parent when variants are involved #1801

Merged
merged 17 commits into from
Jul 24, 2023

Conversation

camertron
Copy link
Contributor

@camertron camertron commented Jul 13, 2023

Fixes primer/view_components#2134

What are you trying to accomplish?

Improve the ability to render the parent's template, taking variants and deeper inheritance hierarchies into account.

Background

A few months ago we introduced a method called #render_parent that, as the name suggests, renders the parent component's template. It is useful in situations where one component inherits from another and it is desirable to render the parent's template within the child's. For example:

<%# app/components/parent.html.erb %>
<div class="parent"></div>

<%# app/components/child.html.erb %>
<div class="child">
  <%= render_parent %>
</div>

Variants

Unfortunately #render_parent doesn't work very smoothly with variants. It assumes all ancestors support the same variants, i.e. respond to the same #call_<variant> methods. In an app where all variants are known this may well be true and desirable, but not so in a component library which may not use variants at all, or may use different names for them (think phone vs mobile, for example).

If the child component supports a variant but the parent does not, ViewComponent currently raises a NoMethodError. Along the same lines, if the child component doesn't support a variant but the parent does, the child will incorrectly render the non-variant template. In fact, one can imagine a 2x2 matrix of such scenarios between any two inherited components:

  1. Self responds to the variant method and so does the parent.
  2. Self responds to the variant method but the parent does not.
  3. Self does not respond to the variant method but the parent does.
  4. Neither self nor the parent respond to the variant method.

As mentioned earlier, one of the components may live in an app and one may live in a library, making the two difficult to modify together.

Deep inheritance hierarchies (i.e. > 2)

Another shortcoming of #render_parent has to do with Ruby's object model. In Ruby, self is always the type it was assigned at instantiation. The only way to move to a different level of inheritance is by calling super, which executes the superclass's method. To every method in the hierarchy however, the type of self is still the original type, and not the super type. Let's look at an example.

class ParentComponent
  def what_am_i
    self.class
  end
end

class ChildComponent < ParentComponent
  def what_am_i
    super
  end
end

ChildComponent.new.what_am_i  # => Child

The method that does the actual work, ParentComponent#what_am_i, sees itself as an instance of ChildComponent despite being defined in the ParentComponent class.

Ruby's behavior in this regard is in contrast to a language like Java, in which objects appear to change their type at each level of the inheritance hierarchy. In Java, the type of self would be ParentComponent inside ParentComponent#what_am_i, even though we actually created an instance of ChildComponent.

Ok, so why is this important? Well, it means that the existing #render_parent method can only work with two levels of inheritance, i.e. a parent-child relationship. It breaks when confronted with a set of grandparent-parent-child relationships.

Let's look at a simplified version of the #render_parent method:

def render_parent
  method(:call).super_method.call
end

Using method(:call) is problematic. It will return the top-most method in the inheritance hierarchy which In our example is ChildComponent#call. For the reason mentioned above, this is true even if we call #method in ParentComponent. Ruby does not provide a way to obtain a reference to the currently executing method, so there's no way to know where we are in the hierarchy, and therefore no way to know who the next parent is.

Said another way, #render_parent finds the #call method on the superclass - but which superclass? Since self is always of type ChildComponent, #render_parent will always call ParentComponent#call. There is no way for it to call GrandparentComponent#call because to do so self would have to be of type ParentComponent... which it can never be.

What approach did you choose and why?

To address the problem, @BlakeWilliams and I modified the existing #render_parent method and made a few changes to the template compiler. Rather than define a #call method, the compiler now defines a method that includes the name of the class it's defined in, eg. #call_parent_component or #call_child_component. Since these methods are unique, they can be called by any member of the inheritance hierarchy to render that specific component's template. This avoids the "which superclass?" problem described earlier. Since the compiler knows exactly which class it's compiling, it can easily find the superclass and thus deduce the method it should call to render the superclass's template.

When inherited, the #render_template_for method defined on the child class is added to an array of unbound methods. As the inheritance hierarchy deepens, the array grows. The array ordering represents the order in which parent templates should be rendered, starting from the top-most child to the bottom-most parent. When a component is rendered, an instance variable counter is set to zero and incremented on each call to #render_parent. This ivar is used to index into the unbound method array.

Crucially, all this can be done without calling super. Special care must be taken to call the correct superclass method considering the current variant, which super cannot do. For example, if the child component does not support the variant but the parent does, #render_parent will render the parent's variant template instead of the default one. In contrast, super is only capable of calling the superclass method by the same name as the current one.

Performance impact

My benchmarks have shown a negligible performance impact.

Co-authored with @BlakeWilliams

Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, although I'm not excited about the extra complexity required to support variants.

IMO we should deprecate them since they don't fit well into component based architectures. Would you mind opening an issue for that as part of this change?

docs/CHANGELOG.md Outdated Show resolved Hide resolved
lib/view_component/compiler.rb Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
@camertron
Copy link
Contributor Author

Re: deprecating variants, see: primer/view_components#2152

docs/api.md Outdated Show resolved Hide resolved
camertron and others added 4 commits July 21, 2023 15:55
Co-authored-by: Blake Williams <blakewilliams@github.com>
…t/view_component into render_parent_with_variants
Copy link
Contributor

@BlakeWilliams BlakeWilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

docs/guide/templates.md Outdated Show resolved Hide resolved
lib/view_component/compiler.rb Outdated Show resolved Hide resolved
camertron and others added 2 commits July 24, 2023 10:18
Co-authored-by: Blake Williams <blakewilliams@github.com>
…hods); add helpful #render_parent_to_string method
@camertron camertron merged commit 425338f into main Jul 24, 2023
30 of 33 checks passed
@camertron camertron deleted the render_parent_with_variants branch July 24, 2023 18:11
claudiob pushed a commit to claudiob/view_component that referenced this pull request Dec 22, 2023
* Fix render_parent when variants are involved

* Ok, yield seems to work

* Got it working for variants and inline templates

* Refactor a bit

* Fix Vale grammar issues; add #render_parent test back in; fix linting issues

* Merge functionality instead

* wip

* Fix tests

* Clean up

* Small fixes

* Update docs/api.md

Co-authored-by: Blake Williams <blakewilliams@github.com>

* Fix linting issues

* Regen docs

* Update lib/view_component/compiler.rb

Co-authored-by: Blake Williams <blakewilliams@github.com>

---------

Co-authored-by: Blake Williams <blake@blakewilliams.me>
Co-authored-by: Blake Williams <blakewilliams@github.com>
claudiob pushed a commit to claudiob/view_component that referenced this pull request Jan 3, 2024
* Fix render_parent when variants are involved

* Ok, yield seems to work

* Got it working for variants and inline templates

* Refactor a bit

* Fix Vale grammar issues; add #render_parent test back in; fix linting issues

* Merge functionality instead

* wip

* Fix tests

* Clean up

* Small fixes

* Update docs/api.md

Co-authored-by: Blake Williams <blakewilliams@github.com>

* Fix linting issues

* Regen docs

* Update lib/view_component/compiler.rb

Co-authored-by: Blake Williams <blakewilliams@github.com>

---------

Co-authored-by: Blake Williams <blake@blakewilliams.me>
Co-authored-by: Blake Williams <blakewilliams@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NavList gives an exception when using rails view variants.
2 participants